Skip to content

Improve YAML marshaling#121

Merged
nathanjcochran merged 5 commits intomainfrom
nathan/improve-yaml-marshaling
Dec 3, 2025
Merged

Improve YAML marshaling#121
nathanjcochran merged 5 commits intomainfrom
nathan/improve-yaml-marshaling

Conversation

@nathanjcochran
Copy link
Member

@nathanjcochran nathanjcochran commented Dec 2, 2025

This PR improves a couple issues with our YAML/JSON marshaling logic, which I noticed when implementing #120.

In particular, the util.SerializeToYAML function previously had an omitNull bool parameter (which was kind of poorly named imo), which would cause it to serialize the results first to JSON, then back into a Go type, and then to YAML. The purpose was to ensure we correctly marshal structs from third-party libraries or generated code that are missing yaml: struct tags, but have json: struct tags (in particular, the omitempty tag, which causes null/empty values to be omitted). However, we were setting the omitNull parameter to true in all but a couple of cases, which meant that the vast majority of our yaml: struct tags were actually being ignored, and were therefore completely unnecessary. It also created a weird footgun, where it you forgot to set omitNull to true, you could end up with YAML output that differed significantly from the corresponding JSON output. I therefore decided to remove the omitNull parameter, make the function always roundtrip to/from JSON first, and get rid of all the yaml: struct tags. This should ensure that our YAML output is always 1:1 with our JSON output (even for third-party or generated structs that only have json: tags) and remove any confusion with regards to the yaml: tags.

One of the only cases where we were setting omitNull to false was when marshaling the config struct for the sake of tiger config show. As a result, the version_check_interval field was being marshaled differently in the JSON vs YAML output. In the JSON output, it was being marshaled as an integer containing nanoseconds (because time.Duration is an int64 under the hood), whereas in the YAML output, it was marshaled using the time.Duration.String() format (due to how gopkg.in/yaml.v3 marshals time.Duration values). Changing the behavior of SerializeToYAML to always roundtrip to/from JSON caused it to use the integer representation everywhere, but I actually think the string representation is the one we want (that's how it's represented in the config file itself, for example, because viper has special handling for durations). I therefore added a special util.Duration type that allows durations to be marshaled to JSON using time.Duration.String(), which make it use the string representation everywhere (i.e. in both the tiger config show -o json and tiger config show -o yaml output).

@nathanjcochran nathanjcochran self-assigned this Dec 2, 2025
"config_dir": tmpDir,
"releases_url": "https://cli.tigerdata.com",
"version_check_interval": float64(3600000000000), // JSON unmarshals time.Duration as nanoseconds (1 hour = 3600000000000ns)
"version_check_interval": "24h0m0s",
Copy link
Member Author

@nathanjcochran nathanjcochran Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even though we had a test that checked for the integer representation when marshaling to JSON, I think it was the wrong behavior. Especially because it didn't match the test for the YAML representation below, which has always used the string representation. I made the JSON and YAML tests essentially 1:1 in this PR.

@nathanjcochran nathanjcochran marked this pull request as ready for review December 3, 2025 18:30
Copy link
Member

@murrayju murrayju left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much improved 🤌

@nathanjcochran nathanjcochran merged commit c0cae55 into main Dec 3, 2025
2 checks passed
@nathanjcochran nathanjcochran deleted the nathan/improve-yaml-marshaling branch December 3, 2025 18:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants